Check if message already delivered #23
Conversation
cam-schultz
requested review from
michaelkaplan13,
minghinmatthewlam,
gwen917,
geoff-vball and
bernard-avalabs
as code owners
| return messageDestinationID == allowedDestinationID | ||
| } | ||
|
|
||
| func isAllowedRelayer(allowedRelayers []common.Address, eoa common.Address) bool { |
There was a problem hiding this comment.
Should relayers be destination specific?
There was a problem hiding this comment.
At the code level, this particular check is ensuring that the relayer EOA that will send the tx to the destination is included in the Teleporter message's list of allowed relayers. The function above checks that the EVM client that will send the tx is sending to the destination chain ID included in the Teleporter message.
At the architectural level, DestinationClient is the entity responsible for sending the tx to the correct destination chain. MessageRelayers should be composed of exactly one DestinationClient, so in that sense, relayers are destination specific.
| zap.String("destinationChainID", destinationChainID.String()), | ||
| zap.String("teleporterMessageID", teleporterMessage.MessageID.String()), | ||
| ) | ||
| return false, nil |
There was a problem hiding this comment.
It looks like the caller should look at the err to distinguished between the message has been received and error conditions. Do the callers do that currently? How should the caller handle errors?
There was a problem hiding this comment.
The call to ShouldSendMessage is valid regardless of the value of the boolean, as long as the error is nil. We perform that check here: https://github.com/ava-labs/awm-relayer/blob/2060ab680cbd1a2501a87ac3ec01d6cd9da79f37/relayer/message_relayer.go#L80-L95
I added a comment to the MessageManager interface noting that the boolean should be ignored if a error is returned.
| m.logger.Info("Relayer EOA not allowed to deliver this message.") | ||
| return false, nil | ||
| } | ||
| if !isDestination(destinationChainID, destinationClient.DestinationChainID()) { |
There was a problem hiding this comment.
This check seems redundant given that we look up the destination client by the chain ID already. If we choose to keep it, could we move this to right after the destinationClient is looked up from the destinationClients map?
There was a problem hiding this comment.
Agreed. I've removed it altogether, but left the corresponding method in DestinationClient, as it makes sense as a getter for that interface.
…/awm-relayer into check-message-already-delivered
cam-schultz
dismissed stale reviews from gwen917 and geoff-vball
via
0a1a368
Why this should be merged
MessageManagerandDestinationClientinterfaces more clearly defined roles.How this works
TeleporterMessengercontract methodmessageReceived(https://github.com/ava-labs/teleporter/blob/main/contracts/src/Teleporter/TeleporterMessenger.sol#L492) inMessageManager:ShouldSendMessage. If the message has already been received, return earlyDestinationClientand intoMessageManager.DestinationClientnow only manages interaction with the destination chain via the relayer EOA, whileMessageManagermanages protocol unpacking and delivery decision logic.How this was tested
CI, Teleporter integration tests, & manual testing.